replacing openssl111 with openssl3#94
replacing openssl111 with openssl3#94gobbledy-gook wants to merge 3 commits intoopen-quantum-safe:mainfrom
Conversation
Signed-off-by: Garvit Shah <garvitshah@Garvits-MacBook-Air.local>
Signed-off-by: Garvit Shah <garvitshah@Garvits-MacBook-Air.local>
Signed-off-by: Garvit Shah <garvitshah@Garvits-MacBook-Air.local>
SWilson4
left a comment
SWilson4
left a comment
There was a problem hiding this comment.
Thanks for the effort to bring this feature up to date, @gobbledy-gook! Is this Docker setup tested anywhere in CI?
| ARG MAKE_DEFINES | ||
|
|
||
| LABEL version="2" | ||
| LABEL version="4" |
There was a problem hiding this comment.
This should probably be version 3.
I'm sorry but I don't know what means, haven't done before. |
No worries! It would be nice to build the Docker image and maybe run a test in it as part of our GitHub Actions Continuous Integration tests. The configuration for these tests is in https://github.com/open-quantum-safe/liboqs-python/blob/main/.github/workflows. I don't believe any of the Docker-related functionality is tested there. I triggered the automated tests to run on this PR, and it looks like there are a couple of build failures to sort out. |
That is a correct observation @SWilson4 and an oversight on my part (well, honestly I wasn't convinced anyone else but me uses docker images for python :), sorry. Unless @gobbledy-gook wants to learn how to do this and add that as part of this PR(?) I'll add a test docker build in a separate PR.
Indeed, the PR failures look like general CI setup problems independent of the PR, so nothing for @gobbledy-gook to worry about. Tagging @vsoftco for advice. |
Indeed. I am open to this. Always eager to expand my scope.
Yes, I observed it being suited for openssl111 and uses python3.10 whereas I have used python3.12 which is the only lowest version that supports openssl3 (afaik). |
Great! As a quick way to get started, you may want to take a look at adding a step to https://github.com/open-quantum-safe/liboqs-python/blob/main/.github/workflows/python_detailed.yml following the docker (test) build in https://github.com/open-quantum-safe/oqs-demos/blob/main/.github/workflows/linux.yml (just without docker hub things like login and push).
Thanks for that information. Should we create a separate issue to track and fix @vsoftco ? |
| home_dir = os.path.expanduser("~") | ||
| oqs_install_dir = os.path.abspath(home_dir + os.path.sep + "_oqs") # $HOME/_oqs | ||
| home_dir = os.path.expanduser("/opt") | ||
| oqs_install_dir = os.path.abspath(home_dir + os.path.sep + "oqssa") # $HOME/_oqs |
There was a problem hiding this comment.
It looks like this change is causing the CI failures—is it necessary?
There was a problem hiding this comment.
I am not sure if this is utterly necessary or not, but it is where the shared object liboqs.so is found by oqs.py.
As I understand (please correct if I am wrong), previously, liboqs.so was being installed during runtime (while oqs.py tries to load the shared object but does not find it and as a result installs at ~/_oqs), however in this case the shared object is required to build ops-provider. Thus now it is installed in ${INSTALLDIR}=/opt/oqssa with all other binaries.
There was a problem hiding this comment.
/opt is a non-standard UNIX path, mostly used on macOS (homebrew). I prefer to have oqs install automatically in the user home directory if not found, so I would not change the initial code above
Yes, please, let's have a separate issue for this. |
vsoftco
force-pushed
the
main
branch
5 times, most recently
from
981bf5c to
7906e78
Compare
4 participants
This is PR has "rough edges" such as some dependencies could be extra or I might have retained extra stuff while building the smaller image in stage 2 or multi-stage build in the dockerfile.
The main goal of this PR is to replace
openssl111withopenssl3which includes the usage of ops-provider. Earlier version of the docker file used python3.8 which did not have support for openssl3 so I had to install the latest python version 3.12 for this purpose. Secondly, theminitest.pyfiles is adjusted to give output forport:6138withsig:dilithium2specifically which requires changing (an easy fix, can be done in a commit).